Release the SIGCHLD watch when the last process event goes - #217
Conversation
libuv_process_event_dispose() deleted its entry from process_events directly, while the teardown lives in libuv_remove_process_event(). An event released while already stopped therefore emptied the table and left the handler armed: nothing raises SIGCHLD again, yet the handler pins the loop, so a worker with no work left never exits. The teardown is now its own function, called from both paths. A hidden timer had the same effect for a different reason. HIDDEN keeps a pool healthcheck out of active_event_count, but uv_timer_start leaves the handle referenced and uv_run does not return. Hidden timers now drop that reference, as the thread-notify handles already did by hand.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Restores the two comments the extraction displaced, and cuts the rest to what the code does not already say.
|
Уточнение по второму фиксу, после разбора механики libuv.
Замер на одинаковой работе — 200 job'ов, 4 потока, healthcheck пула с интервалом 30:
31 тик за секунду работы — примерно раз в 32 мс, то есть таймер тикает как задумано. У сборки без фикса тиков больше только потому, что она молотила ещё 59 секунд после того, как работа кончилась. Попутно: из трёх скрытых таймеров в дереве ( Отдельно от этого PR: |
1 participant
Closes #216.
A worker drains its queue, prints its final stats and then hangs until killed. Under gdb the main thread sits in
uv__io_pollwithtimeout=-1fromlibuv_reactor_execute(no_wait=false); auv_walkdump at that point showsactive_event_countdown to 1 and one live handle —signal,signum=17. No child processes exist, live or zombie.Two causes, both in this file
libuv_process_event_dispose()empties the table behind the teardown's back. It deletes the entry fromASYNC_G(process_events)directly — the line guarding against a stale hash pointer — while the SIGCHLD teardown lives only inlibuv_remove_process_event(). An event released while already stopped leaves the table empty and the handler armed. The teardown is nowlibuv_release_process_watch(), called from both paths.A hidden timer still pins the loop.
pool_start_healthcheck_timer()marks its timer HIDDEN precisely so an idle pool cannot block a graceful shutdown, but that flag only keeps the event out ofactive_event_count.uv_timer_startleaves the handle referenced, souv_runnever returns. The thread-notify handles in this file already work around it with a manualuv_unref; timers now do the same.Measurements
Stand: a Laravel queue worker on thrun threads, 4 threads, jobs that touch the database.
Removing either fix from the final build brings the hang back on the first or second run.
Suite: 1126 pass, the same 3 failures as baseline (
curl/063,curl/064,io/082) — they fail on an unpatched binary too.What is deliberately not here
An earlier draft settled the event on
ECHILDinlibuv_handle_process_events(), on the theory that a child reaped byproc_close()/pclose()leaves the event waiting forever. That branch corrupts exit codes: a notified event stays in the table until its waiter resumes, so a second sweep before that seesECHILDfor the child the first sweep just reaped and overwritesexit_codewith-1. Two children exiting close together while the scheduler runs PHP code — an ordinary shape — turnedproc_close()results of 7 and 9 into -1 and -1, deterministically. A comment now records why that branch must not come back.A draft also polled processes from
libuv_reactor_execute()before the blockinguv_run. Measured unnecessary once the teardown was fixed — 30 clean runs without it — and removed rather than left in the reactor's hot path.Known and unchanged
libuv_handle_process_events()reads the pid out of a copied event pointer before checking that the entry is still in the table. Unreachable today: in scheduler context the notify callbacks only enqueue coroutines, no PHP code runs inline, and every copied event holds two references for the duration of the sweep. It would become reachable if the sweep ever ran outside scheduler context and one waker held two process events. Left as it was — the minimal hardening would be to copy hash keys instead of event pointers.